Skip to content

fix(eda): inherit KiCad reference prefix from ancestor categories - #1547

Open
wangzhengzhuo05 wants to merge 1 commit into
Part-DB:masterfrom
wangzhengzhuo05:fix/kicad-reference-prefix-inheritance
Open

wangzhengzhuo05 wants to merge 1 commit into
Part-DB:masterfrom
wangzhengzhuo05:fix/kicad-reference-prefix-inheritance

Conversation

@wangzhengzhuo05

Copy link
Copy Markdown
Contributor

What

A part in a nested EDA category did not inherit the Reference Prefix of an ancestor category.
KiCadHelper::getKiCADPart() only consulted the part's own EDA info and then its immediate
category, so in the tree below a part placed from D-sub got refdes U? instead of J?:

PCB Components
- Connectors        (Reference Prefix = J)
  - Coaxial
  - D-sub           (blank -> should inherit J)
  - Pin Header

Why

src/Services/EDA/KiCadHelper.php:214 resolved the prefix as
part->edaInfo->prefix ?? part->category->edaInfo->prefix ?? 'U' — the category lookup was
single-level, so any deeper category lost the inherited prefix. Per #1535 the prefix should be
inherited from the next highest category that defines one.

How

Added a private getReferencePrefix(Part $part) helper and used it for the reference field:

  • the part's own prefix still wins;
  • otherwise the category tree is walked upwards, closest ancestor first, and the first category
    that defines a non-empty prefix wins;
  • both null and '' count as "not set", so a blank prefix field inherits (the reported case);
  • the 'U' fallback is unchanged when nothing in the chain defines a prefix;
  • a 20-level depth guard keeps the walk bounded, matching the depth limits already used elsewhere
    in AbstractStructuralDBElement.

Only the reference field is affected — the other EDA fields and the visibility checks are
untouched.

Tests

vendor/bin/phpunit tests/Services/EDA/KiCadHelperTest.php
OK (29 tests, 68 assertions)

Four new tests in tests/Services/EDA/KiCadHelperTest.php:

  • testReferencePrefixIsInheritedFromAncestorCategory — part blank, D-sub blank, parent Connectors = J -> J
  • testNearestCategoryWithPrefixWinsD-sub = X, parent Connectors = J -> X
  • testPartReferencePrefixOverridesCategory — part = C, parent Connectors = J -> C
  • testReferencePrefixFallsBackToUWhenNothingIsSet — nothing set -> U

Mutation check

Restoring line 214 to the previous single-level lookup (leaving the new tests in place) makes the
regression test fail with exactly the reported symptom, then reverting the mutation goes green:

1) ...testReferencePrefixIsInheritedFromAncestorCategory
Failed asserting that two strings are identical.
-'J'
+'U'
Tests: 1, Assertions: 1, Failures: 1.

Fixes #1535

@codecov

codecov Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.40%. Comparing base (d12153a) to head (82bc296).
⚠️ Report is 24 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1547   +/-   ##
=========================================
  Coverage     62.40%   62.40%           
- Complexity     9879     9886    +7     
=========================================
  Files           736      736           
  Lines         31782    31795   +13     
=========================================
+ Hits          19832    19842   +10     
- Misses        11950    11953    +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jbtronics

Copy link
Copy Markdown
Member

I think it would be good that if we do all the recursion, that the result for each category is cached somehow (at least inside the request itself), so that when a part list for categories is created, so the work does not increase linearly with the amount of parts, but something much less.

@wangzhengzhuo05

Copy link
Copy Markdown
Contributor Author

Thanks @jbtronics — closing this.

You are right about the caching point: doing the full ancestor walk per category means the work grows with the number of parts, and caching the per-category result (at minimum for the lifetime of the request) is the right shape. I am not able to follow up on that in the near term, so rather than leave this sitting in your review queue with an open question attached, I am closing it.

For the record, what the branch did: getPartDetails-adjacent EDA code resolved the KiCad reference prefix only from the category it was handed, so a part in a nested category fell back to a generic prefix instead of inheriting the ancestor's (R/C/U). The change walked the ancestor chain to the first category that declares a prefix. That walk is exactly the thing that needs the cache you described.

The branch fix/kicad-reference-prefix-inheritance is still on the fork if anyone wants to pick the direction up (it would also want a rebase — it had gone CONFLICTING against main). The test suite was green locally when it went up. Thanks for the review, and sorry for the extra queue noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reference Prefix parameter inheritance

2 participants